Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Styling ready to claim section. #2335

Merged
merged 5 commits into from
Jan 27, 2022
Merged

Conversation

fairlighteth
Copy link
Contributor

Summary

  • Styles the 'Ready to claim?' section by adding a FAQ drawer component.
  • Styles the "Important!" message below. Intend to use the same component for other user messages (potentially incl. the validation errors).

Desktop

Screen.Recording.2022-01-27.at.16.45.35.mov

Mobile

Screen.Recording.2022-01-27.at.16.46.11.mov

@fairlighteth fairlighteth requested review from a team January 27, 2022 15:49
'By sending this Ethereum transaction, you will be investing tokens from the connected account and exchanging them for vCOW tokens that will be received by the claiming account specified above.',
},
{
title: 'Can I modify (partial) invested amounts later?',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partially?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use partially

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

</p>
<FaqDrawer items={FAQ_DATA} />
<UserMessage>
<SVG src={ImportantIcon} description="Important!" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make this smaller in mobile? and decrease the left/right padding a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will review and tweak.

Copy link
Contributor

@W3stside W3stside left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! probably some iterations we can make on wording but looks good

@elena-zh
Copy link

Hey @biocom , seems like 'Important' section's color is a bit pale and hard to read in the dark mode. WDYT?
image

Then, in real mobile devices (iOS and Android) the cross icon stays focused when close a question:
image

@fairlighteth
Copy link
Contributor Author

@elena-zh

seems like 'Important' section's color is a bit pale and hard to read in the dark mode. WDYT?

Looking at your screenshot indeed 🤔 . Will review.

Then, in real mobile devices (iOS and Android) the cross icon stays focused when close a question:

This is an hover effect. So given on mobile that translates to 'on touch' that's likely why it remains active.

@fairlighteth
Copy link
Contributor Author

@elena-zh @W3stside Addressed both your points:
Screen Shot 2022-01-27 at 18 05 45
Screen Shot 2022-01-27 at 18 05 38

@fairlighteth fairlighteth merged commit d01eaea into release/1.10 Jan 27, 2022
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really great!
I have some heasitation with the red color being to strong, but looks good.

Not from this PR, but if we go for "scary message", shouldn't we mention also that the person will also be investing, or is it too repetitive?
image

}[]
}

export function FaqDrawer({ items }: FaqDrawerProps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome you took the chance to do a reusable component ❤️

@fairlighteth
Copy link
Contributor Author

Not from this PR, but if we go for "scary message", shouldn't we mention also that the person will also be investing, or is it too repetitive?

Yes I think that makes sense, given you only land on this page when you're investing. Should we rename to:

Important! Please make sure you intend to claim/invest in vCOW and send to the above mentioned receiving account.

@elena-zh
Copy link

LTM now!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants